feat(daemon): add configuration loader and wire into startup#2
feat(daemon): add configuration loader and wire into startup#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements milestone M1.4 by adding a comprehensive configuration system to the daemon, enabling runtime customization through environment variables and command-line flags. The changes make the daemon production-ready by replacing hardcoded values with configurable settings for critical parameters like database path, policy path, network binding, polling intervals, and web asset modes.
Changes:
- Added configuration loader with env/flag precedence for daemon settings (DB path, policy path, listen address, poll interval, web asset mode)
- Made API server address and static file handling configurable through new SetAddr and SetStaticFS methods
- Updated documentation (API_SPEC.md, DEPLOYMENT.md, TASKS.md, etc.) to reflect configuration options and mark M1.4 as complete
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/ratelord-d/config.go | New file implementing configuration loading with validation, env/flag parsing, and path normalization |
| cmd/ratelord-d/main.go | Integrated config loader, replaced hardcoded paths with configured values, added web mode switching logic |
| pkg/api/server.go | Added SetStaticFS, SetAddr methods and registerStaticHandler for dynamic configuration |
| TASKS.md | Marked M1.4 as complete |
| PROGRESS.md | Updated file status tracking for modified files |
| PHASE_LEDGER.md | Added M1.4 completion entry |
| NEXT_STEPS.md | Noted M1.4 completion and suggested backlog reassessment |
| LEARNING.md | Added lessons learned from configuration implementation |
| DEPLOYMENT.md | Documented runtime configuration environment variables |
| API_SPEC.md | Added configuration documentation with env var details |
Comments suppressed due to low confidence (1)
cmd/ratelord-d/main.go:185
- The HTTP server is started in a goroutine (line 181) and any startup errors are only logged without terminating the daemon. If the server fails to start (e.g., port already in use), the daemon will continue running but won't serve API requests, which could lead to confusing behavior. Consider either: (1) making server startup blocking until it successfully binds to the port, or (2) adding a health check that verifies the server is actually listening before considering the daemon fully started.
go func() {
if err := srv.Start(); err != nil {
fmt.Printf(`{"level":"error","msg":"server_error","error":"%v"}`+"\n", err)
}
}()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // SetStaticFS sets the filesystem for serving static web assets | ||
| func (s *Server) SetStaticFS(fs fs.FS) { | ||
| s.staticFS = fs | ||
| s.registerStaticHandler() | ||
| } |
There was a problem hiding this comment.
The SetStaticFS method can register a handler dynamically via registerStaticHandler. The http.ServeMux is safe for concurrent reads but handler registration should occur before the server starts serving requests (before ListenAndServe is called). While the current usage in main.go correctly calls SetStaticFS before Start(), the API design allows unsafe usage patterns. Consider either: (1) documenting that SetStaticFS must be called before Start(), or (2) adding synchronization to make it safe to call at any time, or (3) panicking if called after Start() has been invoked.
| func LoadConfig() (Config, error) { | ||
| cwd, err := os.Getwd() | ||
| if err != nil { | ||
| return Config{}, fmt.Errorf("failed to get cwd: %w", err) | ||
| } | ||
|
|
||
| defaultDBPath := filepath.Join(cwd, "ratelord.db") | ||
| defaultPolicyPath := filepath.Join(cwd, "policy.json") | ||
|
|
||
| dbDefault := envOrDefault("RATELORD_DB_PATH", defaultDBPath) | ||
| policyDefault := envOrDefault("RATELORD_POLICY_PATH", defaultPolicyPath) | ||
| listenDefault := defaultListenAddr | ||
| if portOverride := envOrDefault("RATELORD_PORT", ""); portOverride != "" { | ||
| listenDefault = net.JoinHostPort("127.0.0.1", portOverride) | ||
| } | ||
| listenDefault = envOrDefault("RATELORD_LISTEN_ADDR", listenDefault) | ||
| webModeDefault := envOrDefault("RATELORD_WEB_MODE", defaultWebMode) | ||
| webDirDefault := envOrDefault("RATELORD_WEB_DIR", "") | ||
|
|
||
| pollDefault, err := envDuration("RATELORD_POLL_INTERVAL", defaultPollInterval) | ||
| if err != nil { | ||
| return Config{}, err | ||
| } | ||
|
|
||
| dbPath := flag.String("db-path", dbDefault, "Path to the SQLite database") | ||
| policyPath := flag.String("policy-path", policyDefault, "Path to the policy JSON file") | ||
| listenAddr := flag.String("listen-addr", listenDefault, "HTTP listen address") | ||
| pollInterval := flag.Duration("poll-interval", pollDefault, "Provider poll interval (e.g. 10s)") | ||
| webMode := flag.String("web-mode", webModeDefault, "Web assets mode: embedded, dir, off") | ||
| webDir := flag.String("web-dir", webDirDefault, "Web assets directory when web-mode=dir") | ||
|
|
||
| flag.Parse() | ||
|
|
||
| cfg := Config{ | ||
| DBPath: strings.TrimSpace(*dbPath), | ||
| PolicyPath: strings.TrimSpace(*policyPath), | ||
| ListenAddr: strings.TrimSpace(*listenAddr), | ||
| PollInterval: *pollInterval, | ||
| WebMode: strings.TrimSpace(*webMode), | ||
| WebDir: strings.TrimSpace(*webDir), | ||
| } | ||
|
|
||
| return normalizeConfig(cfg) | ||
| } |
There was a problem hiding this comment.
The new LoadConfig function and its helper functions (normalizeConfig, envOrDefault, envDuration) lack test coverage. The cmd/ratelord-d directory has test files (hot_reload_test.go), indicating that testing is expected in this package. Consider adding tests for LoadConfig to cover scenarios such as: environment variable parsing, flag parsing, validation errors (empty paths, invalid poll intervals, invalid web modes), path normalization, and the interaction between RATELORD_PORT and RATELORD_LISTEN_ADDR.
| } | ||
| if cfg.ListenAddr == "" { | ||
| return Config{}, fmt.Errorf("listen address is required") | ||
| } |
There was a problem hiding this comment.
The ListenAddr validation only checks if the address is non-empty, but doesn't validate the format. Consider validating that the listen address is a valid host:port combination using net.SplitHostPort or similar. Invalid addresses will only fail at runtime when the server attempts to start, making misconfiguration harder to debug.
| } | |
| } | |
| if _, _, err := net.SplitHostPort(cfg.ListenAddr); err != nil { | |
| return Config{}, fmt.Errorf("invalid listen address %q: %w", cfg.ListenAddr, err) | |
| } |
| dbPath := flag.String("db-path", dbDefault, "Path to the SQLite database") | ||
| policyPath := flag.String("policy-path", policyDefault, "Path to the policy JSON file") | ||
| listenAddr := flag.String("listen-addr", listenDefault, "HTTP listen address") | ||
| pollInterval := flag.Duration("poll-interval", pollDefault, "Provider poll interval (e.g. 10s)") | ||
| webMode := flag.String("web-mode", webModeDefault, "Web assets mode: embedded, dir, off") | ||
| webDir := flag.String("web-dir", webDirDefault, "Web assets directory when web-mode=dir") | ||
|
|
||
| flag.Parse() |
There was a problem hiding this comment.
Calling flag.Parse() in LoadConfig() means flags can only be parsed once per process lifetime. If LoadConfig is called multiple times (e.g., in tests or future hot-reload scenarios), subsequent calls will fail or behave unexpectedly. Consider checking if flags have already been parsed using flag.Parsed(), or restructuring to separate flag definition from parsing.
| if info, err := os.Stat(cfg.WebDir); err != nil { | ||
| return Config{}, fmt.Errorf("stat web dir: %w", err) | ||
| } else if !info.IsDir() { | ||
| return Config{}, fmt.Errorf("web dir is not a directory: %s", cfg.WebDir) | ||
| } |
There was a problem hiding this comment.
When web_mode is "dir", the config validates that the directory exists and is indeed a directory (lines 112-116). However, this validation happens at startup before the daemon needs to serve assets. If the directory is deleted or becomes inaccessible after the daemon starts, the static file handler will fail at runtime with potentially unclear errors. Consider whether it's preferable to validate directory existence at startup (current behavior) or defer validation until assets are actually served (more resilient to temporary filesystem issues).
| if info, err := os.Stat(cfg.WebDir); err != nil { | |
| return Config{}, fmt.Errorf("stat web dir: %w", err) | |
| } else if !info.IsDir() { | |
| return Config{}, fmt.Errorf("web dir is not a directory: %s", cfg.WebDir) | |
| } |
Motivation
Description
cmd/ratelord-d/config.goimplementingLoadConfig()with env-first defaults, flag overrides, validation/normalization and support forRATELORD_DB_PATH,RATELORD_POLICY_PATH,RATELORD_PORT,RATELORD_LISTEN_ADDR,RATELORD_POLL_INTERVAL,RATELORD_WEB_MODE, andRATELORD_WEB_DIR.cmd/ratelord-d/main.goto use configuredDBPath,PolicyPath,PollInterval,ListenAddr, andWebModewhen initializing store, poller, policy loading, and web assets.pkg/api/server.goby addingSetStaticFS(),SetAddr(), and dynamic registration of the static handler.API_SPEC.md,DEPLOYMENT.md,TASKS.md,PROGRESS.md,PHASE_LEDGER.md,NEXT_STEPS.md, andLEARNING.md.Testing
MISE_DISABLE=1 go test ./...but thegotoolchain was not available in the execution environment so tests could not be run (reported failure due to missinggo).MISE_DISABLE=1 go vet ./...and formatting (gofmt) but these also could not run for the same reason; code was verified by static inspection andgofmt/go vetare listed in the PR testing notes.config_loadedandweb_assets_loadedmessages to aid runtime verification (no automated tests executed due to toolchain limitation).Codex Task